Make extendable configs easier to add #13084

Jarcho · 2024-07-11T03:29:15Z

Two small behaviour changes:

All Vec/HashSet configs can now take ".." as their first item to extend the default (even if there is none).
Only the first or last item can be ".." rather than any item in the list. A quick search showed it mostly used as the final item, rarely as the first, and once appearing in the middle. Ideally this would only support one position.

changelog: All configurations with list can now use ".." in the first or last argument to extend the default. ".." values in the middle of the list will now issue a warning, that they should be moved to the start or end.
#13084

xFrednet

I really like these changes! One tiny nit, then you can r=me :D

clippy_config/src/conf.rs

xFrednet · 2024-07-13T23:32:58Z

Oh one more thing, do we maybe want to add a lint or warning, if the ".." is not in the last index? I feel like it could be confusing otherwise 🤔

Jarcho · 2024-07-13T23:45:06Z

I'm personally on the side that it should be first since this is extending an existing list. An ellipsis comes first when writing so the thing that's acting like one should also come first.

As for linting when it appears in the middle it might be worth it since it's otherwise silently ignored now.

xFrednet · 2024-07-14T09:02:15Z

I'm personally on the side that it should be first since this is extending an existing list. An ellipsis comes first when writing so the thing that's acting like one should also come first.

I think for a single value, the other thing looks a bit better as in

x = ["custom", ".."]

But with multiple values and potentially multiple lines, it's definitely better to have it in the front. So, that should be our default suggestion.

It's probably easier to make this a warning, and not a lint, since we can then just emit it while reading the config file. It'd be good if this warning/lint could be part of this PR, but we could make it a followup PR, if you prefer.

bors · 2024-07-17T19:04:54Z

☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.

Jarcho · 2024-07-18T07:15:28Z

Added a change to the metadata collector. Should this be changed back to printing [] for the default value of an empty array? msrv and trivial_copy_size_limit already didn't list a default value.

For the warning that requires changing how values are deserialized in order to get the span correct. I'll look into it.

xFrednet · 2024-07-18T07:22:27Z

Hmm, I like having the default value for everything for consistency. For booleans etc we also display the default, even if it's the default of those values. So, I'd be in favor of keeping it.

I like that the original order of the values in the default configuration is now retained.

Jarcho · 2024-07-27T20:20:36Z

Latest change is basically a full rewrite using toml_edit instead or serde and toml. This lets us deserialize values with spans and get better error reporting. A bunch of misc. changes are included as well at the moment.

I'll be splitting this up later into multiple commits for easier reading, but you can get a feel for the end result with this. The diff is going to be totally useless right now.

Misc changes to `clippy_config` Contains part of #13084 Changes include: * Sort config list and each configs lint list. * Add default text for the two configs that were missing it. * Switch the lint list in the configs to an attribute. * Make `dev fmt` sort the config list. r? `@xFrednet` changelog: none

xFrednet · 2024-07-30T09:21:19Z

Could you rebase to include the changes from the last PR?

book/src/lint_configuration.md

clippy_config/src/conf.rs

xFrednet

Tiny NIT, the rest looks good to me. We can merge this, once the commit has a proper name and the nit is fixed.

This PR was initially created to restrict the ".." value to the beginning or end. My understanding is that currently all positions are accepted again, right? Adding this restriction and a respective warning can be done in a followup, this PR is already quite large.

xFrednet · 2024-08-01T08:35:49Z

clippy_config/src/de.rs

+macro_rules! deserialize_table {
+    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {


NIT: I think the usage if this macro is not self-explanatory. I would suggest adding a doc comment and renaming it to deserialize_table_row since it only processes one row.

Maybe:

Suggested change

macro_rules! deserialize_table {

($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

/// Can be used to deserialize a table row. Each column has a local

/// of type `Option<T>` that is filled, when the value is found.

///

/// Example usage:

/// ```

/// deserialize_table_row!(dcx, table,

/// path("path"): String,

/// reason("reason"): String,

/// );

/// ```

///

/// Example input:

/// ```toml

/// disallowed-methods = [

/// { path = "std::vec::Vec::leak", reason = "no leaking memory" },

/// # path = Some("std::vec::Vec::leak")

/// # reason = Some("no leaking memory")

///

/// { path = "std::time::Instant::now" },

/// # path = Some("std::vec::Vec::leak")

/// # reason = None

/// ]

/// ```

macro_rules! deserialize_table_row {

($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

What's this about rows? It can deserialize any toml table, both inline and regular. The TableLike trait is implemented for both.

Huh, I guess it depends on what you describe as a "table". I thought the whole disallowed-methods is a table, like this

path reason

"std::vec::Vec::leak" "std::vec::Vec::leak"

"std::time::Instant::now"

But I'm guessing now that you called { path = "std::vec::Vec::leak", reason = "no leaking memory" } a table?

Ah, that's what you mean. The name is referring to a toml table.

Okay, then we can keep it like this. Could you rename the commit and fix the last error? Then we can merge this :D

xFrednet · 2024-08-02T08:35:38Z

Have you seen my question from the last review:

This PR was initially created to restrict the ".." value to the beginning or end. My understanding is that currently all positions are accepted again right? Adding this restriction and a respective warning can be done in a followup, this PR is already quite large.

bors · 2024-08-06T15:13:11Z

☔ The latest upstream changes (presumably #13225) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned xFrednet Jul 11, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 11, 2024

xFrednet approved these changes Jul 13, 2024

View reviewed changes

clippy_config/src/conf.rs Outdated Show resolved Hide resolved

Jarcho mentioned this pull request Jul 17, 2024

Create lint passes using Conf #13088

Merged

Jarcho force-pushed the conf_refactor branch from beda539 to f53fce5 Compare July 17, 2024 22:31

Jarcho mentioned this pull request Jul 28, 2024

Misc changes to clippy_config #13173

Merged

wip

9979380

Jarcho force-pushed the conf_refactor branch from e71254c to 9979380 Compare July 30, 2024 12:26

xFrednet reviewed Jul 31, 2024

View reviewed changes

book/src/lint_configuration.md Outdated Show resolved Hide resolved

clippy_config/src/conf.rs Show resolved Hide resolved

xFrednet approved these changes Aug 1, 2024

View reviewed changes

xFrednet mentioned this pull request Aug 12, 2024

Replace the metadata collector with tests #13221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make extendable configs easier to add #13084

Make extendable configs easier to add #13084

Jarcho commented Jul 11, 2024 •

edited by xFrednet

Loading

xFrednet left a comment

xFrednet commented Jul 13, 2024

Jarcho commented Jul 13, 2024

xFrednet commented Jul 14, 2024

bors commented Jul 17, 2024

Jarcho commented Jul 18, 2024 •

edited

Loading

xFrednet commented Jul 18, 2024

Jarcho commented Jul 27, 2024

xFrednet commented Jul 30, 2024

xFrednet left a comment •

edited

Loading

xFrednet Aug 1, 2024

Jarcho Aug 1, 2024

xFrednet Aug 1, 2024

Jarcho Aug 1, 2024

xFrednet Aug 2, 2024

xFrednet commented Aug 2, 2024

bors commented Aug 6, 2024

		macro_rules! deserialize_table {
		($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

-macro_rules! deserialize_table {
-    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {
+/// Can be used to deserialize a table row. Each column has a local
+/// of type `Option<T>` that is filled, when the value is found.
+///
+/// Example usage:
+/// ```
+/// deserialize_table_row!(dcx, table,
+///     path("path"): String,
+///     reason("reason"): String,
+/// );
+/// ```
+///
+/// Example input:
+/// ```toml
+/// disallowed-methods = [
+///     { path = "std::vec::Vec::leak", reason = "no leaking memory" },
+///     # path   = Some("std::vec::Vec::leak")
+///     # reason = Some("no leaking memory")
+///
+///     { path = "std::time::Instant::now" },
+///     # path   = Some("std::vec::Vec::leak")
+///     # reason = None
+/// ]
+/// ```
+macro_rules! deserialize_table_row {
+    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

path	reason
`"std::vec::Vec::leak"`	`"std::vec::Vec::leak"`
`"std::time::Instant::now"`

Make extendable configs easier to add #13084

Are you sure you want to change the base?

Make extendable configs easier to add #13084

Conversation

Jarcho commented Jul 11, 2024 • edited by xFrednet Loading

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet commented Jul 13, 2024

Jarcho commented Jul 13, 2024

xFrednet commented Jul 14, 2024

bors commented Jul 17, 2024

Jarcho commented Jul 18, 2024 • edited Loading

xFrednet commented Jul 18, 2024

Jarcho commented Jul 27, 2024

xFrednet commented Jul 30, 2024

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

xFrednet Aug 1, 2024

Choose a reason for hiding this comment

Jarcho Aug 1, 2024

Choose a reason for hiding this comment

xFrednet Aug 1, 2024

Choose a reason for hiding this comment

Jarcho Aug 1, 2024

Choose a reason for hiding this comment

xFrednet Aug 2, 2024

Choose a reason for hiding this comment

xFrednet commented Aug 2, 2024

bors commented Aug 6, 2024

Jarcho commented Jul 11, 2024 •

edited by xFrednet

Loading

Jarcho commented Jul 18, 2024 •

edited

Loading

xFrednet left a comment •

edited

Loading